feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #200
Conversation
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSummaryThe Ansible module now collects structured role fingerprints, formats them for syslog and JSONL, and supports locked file output with size trimming. Check mode returns fingerprint data without logging. Unit tests cover collection, formatting, storage, validation, and error handling. ChangesStructured fingerprint logging
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
tests/unit/test_sr_fingerprint.py (3)
195-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a log file that already exceeds
max_log_size.Every trim test grows the file one record at a time, so the file never starts above the limit. That path hides the defect flagged on
library/sr_fingerprint.pyLines 191-198, where_trim_log_filereceives only the new-record size and leaves the existing excess in place.Write several records with trimming disabled, then write one record with a small
max_log_size, and assert that the resulting file size is at or below that limit.💚 Proposed additional test
def test_trim_shrinks_preexisting_oversized_file(self): with tempfile.NamedTemporaryFile(delete=False, suffix=".jsonl") as tmp: log_file = tmp.name try: record = _sample_fingerprint_record() line_size = len(sr_fingerprint._format_fingerprint_jsonl(record) + "\n") for _i in range(10): sr_fingerprint._write_jsonl_log(log_file, record, max_size=0) max_size = line_size * 3 sr_fingerprint._write_jsonl_log(log_file, record, max_size=max_size) self.assertLessEqual(os.path.getsize(log_file), max_size) finally: _cleanup_log(log_file)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_sr_fingerprint.py` around lines 195 - 219, Add a test alongside test_trim_removes_oldest_lines that first writes several records with trimming disabled (max_size=0), then writes one record using a small max_size, and asserts os.path.getsize(log_file) is at or below that limit. Use the existing temporary-file setup, sample record formatting, _write_jsonl_log, and cleanup helpers.
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
shutil.rmtreefor cleanup.If
_write_jsonl_lograises before it createssubdir,os.listdir(subdir)raises insidefinallyand hides the original failure.shutil.rmtreeremoves the tree unconditionally and keeps the real error visible.♻️ Proposed refactor
finally: - subdir = os.path.dirname(log_file) - for name in os.listdir(subdir): - os.unlink(os.path.join(subdir, name)) - os.rmdir(subdir) - os.rmdir(tmpdir) + shutil.rmtree(tmpdir, ignore_errors=True)Add
import shutilto the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_sr_fingerprint.py` around lines 172 - 177, Update the cleanup in the test’s finally block to use shutil.rmtree on the temporary directory, adding the shutil import, so cleanup remains safe when subdir was never created and does not mask the original _write_jsonl_log failure.
352-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the normal syslog path.
No test runs
_handle_fingerprintwithcheck_mode=Falseandwrite_log_file=False. That is the default configuration and the primary documented behavior._FakeModule.loggedis populated at Line 39 but no test reads it, so a regression in themodule.logcall atlibrary/sr_fingerprint.pyLine 320 goes undetected.💚 Proposed additional test
def test_handle_fingerprint_logs_to_syslog_without_log_file(self): module = _FakeModule( { "status": "success", "write_log_file": False, "max_log_size": 2000000, "role_name": "systemd", "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", "ansible_play_hosts_all": ["host1", "host2"], "distribution": "RedHat", "distribution_version": "9.4", }, check_mode=False, ) with self.assertRaises(_ExitJsonException) as ctx: sr_fingerprint._handle_fingerprint(module) self.assertEqual(len(module.logged), 1) self.assertIn("status=success", module.logged[0]) self.assertIn("play_hosts_number=2", module.logged[0]) self.assertFalse(ctx.exception.kwargs["changed"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_sr_fingerprint.py` around lines 352 - 381, Add a unit test alongside test_handle_fingerprint_write_failure_calls_fail_json that invokes _handle_fingerprint with check_mode=False and write_log_file=False, then assert module.logged contains one syslog message including status=success and play_hosts_number=2, and that the resulting _ExitJsonException reports changed=False.library/sr_fingerprint.py (3)
300-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the validated
max_log_sizelocal.Line 300 stores
max_log_size, and Line 326 reads the same parameter again. Pass the local so the validated value and the used value cannot diverge later.♻️ Proposed refactor
- _write_jsonl_log( - log_file, fingerprint_record, module.params["max_log_size"] - ) + _write_jsonl_log(log_file, fingerprint_record, max_log_size)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/sr_fingerprint.py` around lines 300 - 327, The validated max_log_size local is not reused when writing the JSONL log. In the write_log_file branch, update the _write_jsonl_log call to pass max_log_size instead of rereading module.params["max_log_size"], while preserving the existing validation and logging flow.
283-287: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe syslog quoting convention is unpinned and untested.
_format_fingerprint_key_valuedoubles an embedded"(CSV style) while commonkey=valuelog parsers expect backslash escaping, and it never escapes\. No test covers a value that contains",=, or\, so the escaping branch is unverified. The PR adds these records for downstream consumers, so fix the convention before release.
library/sr_fingerprint.py#L283-L287: choose one documented convention and implement it; for logfmt, escape\and"with a backslash and add\to the trigger characters.tests/unit/test_sr_fingerprint.py#L131-L140: add assertions for values that contain",=, and\, matching the chosen convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/sr_fingerprint.py` around lines 283 - 287, Update _format_fingerprint_key_value in library/sr_fingerprint.py:283-287 to use the documented logfmt convention, triggering quoting for backslashes as well as spaces, equals signs, and quotes, and escaping both backslashes and quotes with a backslash. Add assertions in tests/unit/test_sr_fingerprint.py:131-140 covering values containing ", =, and \ and matching this convention.
193-208: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead and write the log in binary mode.
Line 193 opens the file in text mode. Two problems follow:
- Line 197 measures
len(line)in characters, not bytes. The size limit is a byte limit. Any multi-byte content in the file makes the accounting wrong. Records written by this module are ASCII-escaped, but a pre-existing or externally written log is not guaranteed to be ASCII.- A non-decodable byte raises
UnicodeDecodeError. That is aValueError, not anOSError, so the handler at Line 328 does not catch it, and the module fails with a traceback instead offail_json.Use binary mode for both the read and the temporary write.
♻️ Proposed refactor
- with open(log_file, "r") as log_fd: + with open(log_file, "rb") as log_fd: lines = log_fd.readlines() @@ - with os.fdopen(fd, "w") as tmp_fd: + with os.fdopen(fd, "wb") as tmp_fd: tmp_fd.writelines(lines)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/sr_fingerprint.py` around lines 193 - 208, Update the log rotation logic around the open/read and temporary-file write operations to use binary mode throughout. Preserve raw bytes when loading and writing lines so size_removed uses byte lengths, arbitrary non-decodable content is supported, and the existing fail_json error handling remains effective.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/sr_fingerprint.py`:
- Around line 191-198: Update the caller of _trim_log_file in
library/sr_fingerprint.py (lines 191-198) to pass the full size deficit,
cur_size plus len(new_line) minus max_size, so oversized files are trimmed until
they fit; add a regression test in tests/unit/test_sr_fingerprint.py (lines
195-219) that creates several records with max_size=0, appends one record using
a small max_log_size, and asserts the resulting file size does not exceed that
limit.
In `@tests/unit/test_sr_fingerprint.py`:
- Line 17: Update the test runner configuration used by
tests/unit/test_sr_fingerprint.py so PYTHONPATH includes library, allowing the
import sr_fingerprint to resolve in CI. Prefer adding PYTHONPATH=library to the
existing tox.ini test command without changing the test itself.
---
Nitpick comments:
In `@library/sr_fingerprint.py`:
- Around line 300-327: The validated max_log_size local is not reused when
writing the JSONL log. In the write_log_file branch, update the _write_jsonl_log
call to pass max_log_size instead of rereading module.params["max_log_size"],
while preserving the existing validation and logging flow.
- Around line 283-287: Update _format_fingerprint_key_value in
library/sr_fingerprint.py:283-287 to use the documented logfmt convention,
triggering quoting for backslashes as well as spaces, equals signs, and quotes,
and escaping both backslashes and quotes with a backslash. Add assertions in
tests/unit/test_sr_fingerprint.py:131-140 covering values containing ", =, and \
and matching this convention.
- Around line 193-208: Update the log rotation logic around the open/read and
temporary-file write operations to use binary mode throughout. Preserve raw
bytes when loading and writing lines so size_removed uses byte lengths,
arbitrary non-decodable content is supported, and the existing fail_json error
handling remains effective.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 195-219: Add a test alongside test_trim_removes_oldest_lines that
first writes several records with trimming disabled (max_size=0), then writes
one record using a small max_size, and asserts os.path.getsize(log_file) is at
or below that limit. Use the existing temporary-file setup, sample record
formatting, _write_jsonl_log, and cleanup helpers.
- Around line 172-177: Update the cleanup in the test’s finally block to use
shutil.rmtree on the temporary directory, adding the shutil import, so cleanup
remains safe when subdir was never created and does not mask the original
_write_jsonl_log failure.
- Around line 352-381: Add a unit test alongside
test_handle_fingerprint_write_failure_calls_fail_json that invokes
_handle_fingerprint with check_mode=False and write_log_file=False, then assert
module.logged contains one syslog message including status=success and
play_hosts_number=2, and that the resulting _ExitJsonException reports
changed=False.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cbe26997-0e8a-4ba0-8e4e-b1922d2c4c1b
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
| def _trim_log_file(log_file, size_needed): | ||
| """Remove oldest records until the file can accommodate size_needed bytes.""" | ||
| with open(log_file, "r") as log_fd: | ||
| lines = log_fd.readlines() | ||
| size_removed = 0 | ||
| while lines and size_removed < size_needed: | ||
| size_removed += len(lines.pop(0)) | ||
| orig_stat = os.stat(log_file) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
max_log_size is not enforced for an already-oversized file. The caller passes only the new-record size as size_needed, so _trim_log_file removes room for one record instead of removing the current excess. The unit tests only grow the file one record at a time, so they never reach the state that exposes the gap.
library/sr_fingerprint.py#L191-L198: pass the real deficit,cur_size + len(new_line) - max_size, so the loop trims until the file fits undermax_size.tests/unit/test_sr_fingerprint.py#L195-L219: add a test that writes several records withmax_size=0, then writes one record with a smallmax_log_size, and asserts the resulting file size is at or below that limit.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 192-192: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_file, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
📍 Affects 2 files
library/sr_fingerprint.py#L191-L198(this comment)tests/unit/test_sr_fingerprint.py#L195-L219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/sr_fingerprint.py` around lines 191 - 198, Update the caller of
_trim_log_file in library/sr_fingerprint.py (lines 191-198) to pass the full
size deficit, cur_size plus len(new_line) minus max_size, so oversized files are
trimmed until they fit; add a regression test in
tests/unit/test_sr_fingerprint.py (lines 195-219) that creates several records
with max_size=0, appends one record using a small max_log_size, and asserts the
resulting file size does not exceed that limit.
| import tempfile | ||
| import unittest | ||
|
|
||
| import sr_fingerprint |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the test configuration and check for library/ on the path.
fd -H -t f 'tox.ini|pytest.ini|setup.cfg|pyproject.toml|conftest.py' \
--exec rg -n 'PYTHONPATH|sys\.path|pythonpath|testpaths|library' {} \;Repository: linux-system-roles/postgresql
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked config/test helper files:"
git ls-files | rg '(^tox\.ini$|^pytest\.ini$|^setup\.cfg$|^pyproject\.toml$|^conftest\.py$|^tests/unit/)' | sed -n '1,120p'
echo
echo "All tracked files mentioning pytest/import/sr_fingerprint/pythonpath:"
rg -n 'pytest|importlib|sys\.path|PYTHONPATH|pythonpath|sr_fingerprint' -S --glob '!*.pyc' . | sed -n '1,200p'
echo
echo "tests/unit files (names only):"
git ls-files 'tests/unit/*' | sed -n '1,120p'Repository: linux-system-roles/postgresql
Length of output: 4794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tox.ini:"
cat -n tox.ini
echo
echo "Python path-related searches in tox.ini:"
rg -n 'envlist|commands|setenv|pythonpath|PYTHONPATH|pytest|testpaths|addopts' tox.ini || true
echo
echo "Project-level files:"
git ls-files | rg '(^|/)(README\.md|README\.rst|tox\.ini|pytest\.ini|setup\.cfg|pyproject\.toml|Makefile|\.ci|\.github(/.*|))|(^\.ci/|(^|/)\.github/.*)' | sed -n '1,200p'
echo
echo "test files contents around imports:"
sed -n '1,80p' tests/unit/test_sr_fingerprint.py | cat -nRepository: linux-system-roles/postgresql
Length of output: 3709
Set the unit-test runner path for sr_fingerprint.
tests/unit/test_sr_fingerprint.py imports library/sr_fingerprint.py directly, but tox.ini does not define the Python path. Add PYTHONPATH=library to the test command or a runner config so the test runs in CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_sr_fingerprint.py` at line 17, Update the test runner
configuration used by tests/unit/test_sr_fingerprint.py so PYTHONPATH includes
library, allowing the import sr_fingerprint to resolve in CI. Prefer adding
PYTHONPATH=library to the existing tox.ini test command without changing the
test itself.
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users. Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl Signed-off-by: Sergei Petrosian <spetrosi@redhat.com>
8a69512 to
28c15d2
Compare
The sr_fingerprint module was rewritten to accept structured parameters (status, role_name, role_path, etc.) instead of a free-form sr_message. Update the role tasks and tests to match the new module interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[citest] |
2 participants
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip]
Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users.
Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl